Clarify print output to specify posterior draws and log-likelihood terms#328
Clarify print output to specify posterior draws and log-likelihood terms#328ishaan-arora-1 wants to merge 3 commits intostan-dev:masterfrom
Conversation
The previous print output "Computed from 4000 by 262 log-likelihood matrix" was ambiguous about which dimension was draws and which was observations. This made it easy for users to miss cases where they accidentally misspecified the log-likelihood (e.g. placing all observations in one multivariate normal). The new output explicitly labels both dimensions: "Computed from 4000 posterior draws and 262 log-likelihood terms." Updated all print_dims methods (psis_loo, importance_sampling, importance_sampling_loo, waic, psis_loo_ss, elpd_generic), along with corresponding tests, snapshots, and vignette output. Fixes stan-dev#198
|
Hi @ishaan-arora-1, can you clarify why did you close this PR? Based on a quick look, it seems to be useful |
Hey, i just wanted to review, why the tests weren't passing before reopening it. I am happy that this pr sounds useful. i'll try to get the tests working in a few hours. |
|
In some of our repos there have been some CI tests failing due to Windows problems. It may take some days before your recent PRs are reviewed. Thanks for submitting them |
|
Glancing at the failed logs, I think tests need to be upated to reflect the new documentation. |
|
do you maybe want a hand in that? just lemme know, ill be happy to contribute.
|
|
Yes, do you want to take a stab at it? The tests are expecting "subsampled log-likelihood values" but the new message is "subsampled log-likelihood terms", so the test case should reflect that. |
sure, would love to :) |
The print_dims.psis_loo_ss method now says "log-likelihood terms" instead of "log-likelihood values", so update the test expectations to match.
|
Fixed! The tests in Ran the full test suite locally, 1160 pass, 0 fail, 0 warnings (2 skips are the usual M1 Mac platform skips). Any feedback @avehtari @VisruthSK |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #328 +/- ##
=======================================
Coverage 92.78% 92.78%
=======================================
Files 31 31
Lines 2992 2993 +1
=======================================
+ Hits 2776 2777 +1
Misses 216 216 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Lgtm. If Aki likes the language I think this is good to merge. The only thing I can think of is maybe the tests that you just had to fix should be snapshot tests instead, but that change needn't be made in this PR. |
sounds good, i can do that in another pr. |
|
@avehtari any comments? |
I like this one.
I don't know right away how this should be said, but subsampling loo uses 1) faster but more biased computation with 4000 posterior draws and 3020 log-likelihood terms and 2) slower but more accurate computation with 4000 posterior draws and 100 subsampled log-likelihood terms |
|
The text for subsampling case in the latest commit is too long. For example, with the example in tests, the text is 225 characters. 1) With Before making any additional commits, make a proposal for new text here in the discussion, and after we have agreed on good new text, then you can make a commit for that. This will save you time, as you don't need to change the tests before we have agreed on the text. |
4 participants
Hey! This addresses #198, where the current print output like
was flagged as ambiguous — it's not immediately clear which dimension is the number of posterior draws and which is the number of observations/log-likelihood terms. As mentioned in the issue, ParetoSmooth.jl already uses a more explicit format, and this kind of ambiguity can hide bugs (e.g. a user accidentally placing all observations in one multivariate normal).
Changes
The new output format is:
This is close to what @avehtari proposed in the issue comments (
"Computed from 4000 posterior draws by 262 log-likelihood terms matrix"), but I dropped "matrix" since the user doesn't really need to know it's a matrix — they care about what the numbers mean.For log-weight objects (raw
psis/tis/sis):For subsampled loo:
Updated methods
print_dims.psis_looprint_dims.importance_samplingprint_dims.importance_sampling_looprint_dims.waicprint_dims.psis_loo_ssprint_dims.elpd_genericAlso updated
test_print_plot.Randtest_loo_subsampling_cases.Rpsis.md,tisis.md, andloo_moment_matching.mdloo2-with-rstan.Rmdandloo2-large-data.RmdNEWS.mdentryFixes #198